Skip to content

Report batches that were never sealed instead of completing them - #13

Draft
jpcamara wants to merge 4 commits into
mainfrom
batch-never-fabricate-completion
Draft

Report batches that were never sealed instead of completing them#13
jpcamara wants to merge 4 commits into
mainfrom
batch-never-fabricate-completion

Conversation

@jpcamara

@jpcamara jpcamara commented Aug 24, 2026

Copy link
Copy Markdown
Owner

Draft — against my own fork for discussion, not upstream.

Maintenance could complete a batch whose transaction was still open or rolled back — firing on_success for work that never happened and losing the deferred enqueues to AlreadyFinished. This PR makes that impossible, without giving up crash recovery.

The three scenarios, as integration tests in batch_lifecycle_test.rb (each runs a real dispatcher and worker):

1. Transaction outlives the stalled window → now works

JobResult.transaction do
  JobResult.create!(...)
  batch = SolidQueue::Batch.enqueue(on_success: BatchOnSuccessJob.new("late")) do
    AddToBufferJob.perform_later("late")            # deferred until commit
  end

  SolidQueue::Batch.where(id: batch.id).update_all(created_at: 10.minutes.ago)
  SolidQueue::Batch.sweep_stalled                   # maintenance on another process

  assert_not batch.reload.finished?                 # ← was: finished, on_success fired
end

wait_for_batches_to_finish_for(5.seconds)
assert_equal [ "late", "late: 1 jobs succeeded!" ], JobBuffer.values.sort   # ← was: AlreadyFinished at commit

2. Rollback → no phantom success

JobResult.transaction do
  batch = SolidQueue::Batch.enqueue(on_success: BatchOnSuccessJob.new("phantom")) do
    AddToBufferJob.perform_later("rolled back")
  end
  raise ActiveRecord::Rollback
end

SolidQueue::Batch.where(id: batch.id).update_all(created_at: 10.minutes.ago)
SolidQueue::Batch.sweep_stalled

assert_not batch.reload.finished?                   # reported now, removed after a day
assert_not_includes JobBuffer.values, "phantom: 1 jobs succeeded!"   # ← was: fired

3. Server dies right after enqueue → nothing to repair

SolidQueue::Batch.any_instance.stubs(:start)        # the deferred start never runs

batch = SolidQueue::Batch.enqueue(on_success: BatchOnSuccessJob.new("sealed")) do
  AddToBufferJob.perform_later("sealed")
end

assert batch.reload.enqueued?                       # sealed in the same commit as its row

wait_for_batches_to_finish_for(5.seconds)
assert_equal [ "sealed", "sealed: 1 jobs succeeded!" ], JobBuffer.values.sort

How

Two changes:

Maintenance completes only batches their creator sealed (called start on). Unsealed ones follow a lifecycle instead of a guess:

  • 5 minutes: reported — a stalled_batch.solid_queue event each, visible via SolidQueue::Batch.stalled, resolvable early with .start or .destroy.
  • 1 day (expire_after): removed. A batch still unsealed then came from a transaction that will never commit; its data rolled back, so the batch ends the same way — as if never created. No callbacks, no accumulation, nothing to monitor.

Jobs that reached the queue before a rollback already ran and stay untouched — the same behaviour as non-batch jobs enqueued in a rolled-back transaction. A transaction that outlives the expiry and then commits raises AlreadyFinished from its deferred enqueues: loud, and without success callbacks over lost work.

A batch created outside any transaction seals itself atomically:

seal_on_create = ActiveRecord.all_open_transactions.none?   # before opening our own txn

transaction do
  save! if new_record?
  wrap_in_batch_context(id) { block.call }
  seal_if_filled if seal_on_create    # UPDATE ... SET enqueued_at WHERE total_jobs != 0
end

Crash → either nothing committed or a sealed batch and its jobs committed together, which is why scenario 3 needs no sweeper.

The total_jobs != 0 guard: deferred enqueues put no jobs in the batch's own transaction, and a sealed batch with nothing outstanding means complete — sealing there would let a concurrent sweep finish it before the deferred INSERTs land. So all-deferred configurations (Rails 7.2 :default, 8.2 defaults) keep sealing via the deferred start; a crash in that window goes to the reported path, since the jobs existed only in process memory and there is nothing durable to recover.

Rails 7.1 (no all_open_transactions) is unchanged.

Verification

Suites green on Rails 7.1 / 7.2 / 8.0 / 8.1 / main, PostgreSQL, shared-pool and separate-pool layouts, rubocop clean. Companion: #7 deletes the leftover pending rows from scenario 2.

🤖 Generated with Claude Code

jpcamara and others added 2 commits August 24, 2026 19:57
Batch maintenance treated any batch older than the stalled window that had
never been started as a process that died mid-creation, and started it on
its behalf. Started with nothing outstanding, the batch immediately
finished and fired its callbacks.

That guess is wrong as often as it's right. An unsealed batch is one of two
things, and nothing in the queue database tells them apart: a creator that
died before sealing it, or a creator still filling it from a transaction
that hasn't committed. Active Job defers those enqueues until commit, and
with a separate queue database the batch row doesn't wait for them, so a
transaction that outlives the window leaves a batch that looks abandoned
and isn't. Completing it finishes the batch over whatever happened to have
landed, fires on_success for work that never ran, and leaves the real
enqueues to raise AlreadyFinished.

Stop guessing. Complete only batches whose creator sealed them by calling
start, since sealed is what makes an empty batch meaningfully complete
rather than merely unfilled. Report the rest through a stalled_batch event
and a Batch.stalled scope, and let an operator adopt one with Batch#start
once they've established its creator is gone.

This trades automatic recovery of genuinely crashed batches for never
reporting success over work that didn't happen. Their jobs still run
either way; only the batch's own completion waits for a decision.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reporting unsealed batches instead of completing them left genuinely
crashed creators to manual adoption. Narrow that: when Batch#enqueue runs
with no transaction open anywhere, nothing outside it can roll the batch
back or defer its jobs, so seal in the same transaction as the batch row.
Either nothing commits, or a sealed batch and its jobs commit together—a
creator dying mid-creation can no longer leave an unsealed batch behind,
and crash recovery needs no operator.

Only batches that received jobs in their own transaction seal this way. A
batch with none may still be waiting on enqueues deferred to a commit that
hasn't happened, and a sealed empty batch means complete, so those keep
deferring to start.

An unsealed batch therefore came from inside a transaction, or is waiting
on deferred enqueues: exactly the cases where completing it loses work,
which is why the sweep reports rather than adopts them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jpcamara

Copy link
Copy Markdown
Owner Author

Revised per discussion: batches created outside any transaction now seal atomically with their own row, so a server dying mid-creation can't strand one — automatic crash recovery is back for that case without the sweeper ever guessing. The manual path now only covers batches created inside a transaction (where completing them was the data-loss bug) and all-deferred configurations. PR body rewritten to match.

jpcamara and others added 2 commits August 24, 2026 23:00
A transaction that outlives the stalled window, a rollback, and a creator
dying right after enqueue: one test each, end to end with a dispatcher
and worker, asserting what actually runs and what never fires.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reporting unsealed batches left an expectation problem: nobody expects
"sometimes batches get stuck forever, and if you don't monitor an event
you'll accumulate rows that never complete" as default behaviour.

A batch still unsealed a day after creation came from a transaction that
will never commit. Its data rolled back, so the batch ends the same way:
the sweep removes it, as if it was never created. No callbacks fire over
rolled-back work, and nothing accumulates or needs monitoring. Jobs that
reached the queue before the rollback already ran and stay untouched,
matching jobs enqueued outside a batch in a rolled-back transaction.

Between the stalled report at five minutes and expiry at a day, batches
remain visible through Batch.stalled for anyone who wants to complete or
discard them early. A transaction that outlives the expiry and then
commits finds its batch gone and raises AlreadyFinished from its deferred
enqueues: loud, and without success callbacks over lost work.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jpcamara

Copy link
Copy Markdown
Owner Author

Addressed the expectations problem: unsealed batches no longer sit around forever waiting for someone to notice an event. They now expire — still unsealed a day after creation means the transaction is never committing, so the sweep removes the batch as if it was never created. Rolled-back batch data and its batch row now end the same way. Nothing to monitor by default; Batch.stalled remains for anyone who wants to look between the 5-minute report and expiry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant